refactor: clean protos for clarity #1071

shaspitz · 2023-06-22T20:27:15Z

Description

Closes: #1072

Achieves:

Properly organizing .proto files.
Rebuilding .pb.go files accordingly.
Changing package references accordingly.

Note this PR also contributes toward #801, in that we're reducing the amount of cross dependancies between the consumer ccv module and the provider ccv module. The end goal is that all shared dependancies live within x/ccv/types, then x/ccv/provider and /x/ccv/consumer do no reference one another. This is surely possible, but not easy and a huge source of tech debt.

Notes:

protobuf break check is expected to fail.
If this PR was implemented correctly, it should be a pure refactor, not changing any code functionality.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

shaspitz · 2023-06-22T20:34:52Z

proto/interchain_security/ccv/v1/shared_consumer.proto

+import "google/protobuf/timestamp.proto";
+
+//
+// Note any type defined in this file is referenced/persisted in both the consumer and provider CCV modules,


TLDR most consumer protos are persisted on provider, so most ended up in this file

All these consumer protos are nil for new consumer chains and only populated in the consumer genesis state when the chain is restarted (on export/import). IIRC, the provider only needs to store the consumer genesis for new chains. It would be good to differentiate between this two scenarios and have all these ptotos local to the consumer.

I see where you're coming from, but from a pure dependency perspective, the consumer's genesis state struct is referenced by both the consumer and provider modules. Any type that's a field in that struct must also be referenced by both modules, even if nil.

There's some ways we could untangle these cross dependancies, like creating a new struct that'd be spit out of MakeConsumerGenesis for the provider, and making that a field in the consumer's true GenesisState struct. But there's complexities in how the consumer genesis JSON would be constructed.

This PR is mostly intended to illuminate dep issues rather than fix them all, and it should stay as a true refactor

proto/interchain_security/ccv/v1/shared_consumer.proto

shaspitz · 2023-08-15T19:32:22Z

x/ccv/consumer/types/genesis_test.go

-				ccv.ConsumerPacketDataList{},
-				types.LastTransmissionBlockHeight{},
-				false,
+				Params:                      params,


I was required to add keys to struct fields in this file to appease linter

shaspitz · 2023-08-15T19:40:21Z

@mpoke @MSalopek does a change like this warrant incrementing the protobuf version number from v1? See proto package names like interchain_security.ccv.consumer.v1. Imo we should increment the version as this is a meaningful change to how protos are defined. The only possible issue that could arise is if the pb version is somehow serialized to metadata sent over the wire.

mpoke

Nice work @smarshall-spitzbart. I really like the idea of this. See my comments bellow.

proto/interchain_security/ccv/v1/shared_consumer.proto

mpoke · 2023-08-16T12:46:20Z

proto/interchain_security/ccv/v1/shared_consumer.proto

+import "google/protobuf/timestamp.proto";
+
+//
+// Note any type defined in this file is referenced/persisted in both the consumer and provider CCV modules,


All these consumer protos are nil for new consumer chains and only populated in the consumer genesis state when the chain is restarted (on export/import). IIRC, the provider only needs to store the consumer genesis for new chains. It would be good to differentiate between this two scenarios and have all these ptotos local to the consumer.

proto/interchain_security/ccv/provider/v1/provider.proto

mpoke · 2023-08-16T13:17:28Z

x/ccv/provider/keeper/genesis.go

@@ -158,7 +158,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
 		k.GetAllValsetUpdateBlockHeights(ctx),
 		consumerStates,
 		k.GetAllUnbondingOps(ctx),
-		&ccv.MaturedUnbondingOps{Ids: k.GetMaturedUnbondingOps(ctx)},
+		&types.MaturedUnbondingOps{Ids: k.GetMaturedUnbondingOps(ctx)},


Use cvvtypes for more clarity. Please apply throughout the repo.

I fully agree that we should use the ccvtypes alias everywhere, instead of types or ccv. However I don't think that sort of change is appropriate for this PR as we're already at 4k lines of changes.

Making the alias change in this file would add ~14 lines from the look of it, but other files have even more refs to the undesirable aliases

insumity · 2023-08-21T11:02:06Z

x/ccv/consumer/ibc_module_test.go

@@ -262,7 +261,7 @@ func TestOnChanOpenAck(t *testing.T) {
 		{
 			"invalid: mismatched serialized version",
 			func(keeper *consumerkeeper.Keeper, params *params, mocks testkeeper.MockedKeepers) {
-				md := providertypes.HandshakeMetadata{
+				md := ccv.HandshakeMetadata{


Why do we have ccv "github.com/cosmos/interchain-security/v3/x/ccv/types" here and not simply"github.com/cosmos/interchain-security/v3/x/ccv/types" as in ibc_module.test.go. Does this lead to inconsistencies between ibc_module.go and ibc_module_test.go, e.g., we have ccv.HandshakeMetadata here instead of types.HandshakeMetadata in ibc_module.go?

See #1071 (comment), the type aliases in this repo are very inconsistent and this does lead to inconsistencies, but I don't think it's worth fixing all in this PR

👍 I agree. We can create another PR for fixing naming inconsistencies across all the files, just so we don't forget.

insumity · 2023-08-21T11:06:16Z

x/ccv/consumer/keeper/distribution_test.go

@@ -12,6 +12,7 @@ import (

 	testkeeper "github.com/cosmos/interchain-security/v3/testutil/keeper"
 	"github.com/cosmos/interchain-security/v3/x/ccv/consumer/types"
+	ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types"


Similarly to my previous comment, there seems the naming is consistent between distribution.go and distribution_test.go.

proto/interchain_security/ccv/v1/wire.proto

insumity · 2023-08-21T11:22:17Z

proto/interchain_security/ccv/v1/wire.proto

+message HandshakeMetadata {
+  string provider_fee_pool_addr = 1;
+  string version = 2;
+}


Is my understanding correct that the reward distribution is performed through a MsgTransfer message and hence why we do not see something reward-related in this file? If yes, it might make sense to add a comment here to point to this.

Yea consumer rewards are sent through a distribution channel, whereas wire.proto is for schemas sent over a ccv channel, this comment can hopefully help

insumity · 2023-08-21T11:28:36Z

proto/interchain_security/ccv/v1/shared_consumer.proto

+import "google/protobuf/timestamp.proto";
+
+//
+// Note any type defined in this file is referenced/persisted in both the consumer and provider CCV modules,


nit: reading this referenced/persisted in both the consumer and provider CCV modules, I would have expected the file to be called shared.proto. My understanding is the file is called shared**_consumer**.proto because it contains only consumer-related types used by the consumer but also referenced by the provider. If that's the case, might make sense to clarify in the comment.

In this scenario, the file contains only consumer-related types used by the consumer but also referenced by the provider. This happens to also be the same set of types that are more generally shared between consumer/provider.

I can add a comment with this sort of this thing, or maybe it's worth clarifying once #1214 is completed

👍 So, I guess if this file would radically change as part of #1214, then we can skip any changes here.

insumity · 2023-08-22T15:28:22Z

x/ccv/consumer/keeper/params.go

@@ -6,8 +6,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

-	"github.com/cosmos/interchain-security/v3/x/ccv/consumer/types"
-	ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types"
+	types "github.com/cosmos/interchain-security/v3/x/ccv/types"


Related to the comment. I guess in these cases we can use ccvtypes even in this PR since the name is already changed?

Well the code that was previously in "github.com/cosmos/interchain-security/v3/x/ccv/consumer/types" is now in "github.com/cosmos/interchain-security/v3/x/ccv/types", so originally using the types alias here avoids a big diff. But I'll make the change since we're already talking about it 065c2cd

insumity · 2023-08-22T15:28:39Z

x/ccv/consumer/types/genesis_test.go

@@ -16,8 +16,7 @@ import (
 	tmtypes "github.com/cometbft/cometbft/types"

 	"github.com/cosmos/interchain-security/v3/testutil/crypto"
-	"github.com/cosmos/interchain-security/v3/x/ccv/consumer/types"
-	ccv "github.com/cosmos/interchain-security/v3/x/ccv/types"
+	types "github.com/cosmos/interchain-security/v3/x/ccv/types"


Related to the comment. I guess in these cases we can use ccvtypes even in this PR since the name is already changed?

Making the alias change here would increase PR diff size by 122 lines, so I'd again encourage this for a separate PR

insumity

I left some additional nit comment, but in general looks good to me, so approving.
Thanks for working on this Shawn!

* docs: add testing improvements ADR (adr-011) (#1197) * docs: add testing improvements ADR * docs: fix links and typos * Update docs/docs/adrs/adr-011-improving-test-confidence.md Co-authored-by: Shawn <[email protected]> * Update docs/docs/adrs/adr-011-improving-test-confidence.md Co-authored-by: Shawn <[email protected]> * docs: update from comments --------- Co-authored-by: Shawn <[email protected]> * refactor: clean protos for clarity (#1071) * consumer proto folder * provider comments, move one msg * ccv comments * moved shiz * proto changes to build * make proto gen again * no circ dep * Update wire.proto * comments * dun builds * progress save * FIN * refactors * rm improper proto ref * rm todos, issue now * lint till I die * update comments, rebuild protos * change ccv.go to wire.go, and test file * consistent comment * Update wire.proto * example alias diff --------- Co-authored-by: MSalopek <[email protected]>

shaspitz added 7 commits June 22, 2023 12:15

consumer proto folder

09146f2

provider comments, move one msg

1ed4a86

ccv comments

9f39733

moved shiz

cc9a354

proto changes to build

98fd486

make proto gen again

c493387

no circ dep

0974bdc

shaspitz commented Jun 22, 2023

View reviewed changes

proto/interchain_security/ccv/v1/shared_consumer.proto Outdated Show resolved Hide resolved

shaspitz commented Jun 22, 2023

View reviewed changes

proto/interchain_security/ccv/v1/shared_consumer.proto Outdated Show resolved Hide resolved

shaspitz mentioned this pull request Jun 22, 2023

Unorganized proto definitions #1072

Closed

shaspitz added 7 commits August 14, 2023 11:56

Merge branch 'main' into shawn/clean-protos

2c1361e

Update wire.proto

d652827

comments

41f60d4

dun builds

ea1bb3e

progress save

4a5a561

FIN

61ea661

Merge branch 'main' into shawn/clean-protos

f6ab871

shaspitz mentioned this pull request Aug 15, 2023

Vaguely named consumer structs #1206

Closed

shaspitz added 5 commits August 15, 2023 11:47

refactors

f267c8a

Merge branch 'main' into shawn/clean-protos

6623a5f

rm improper proto ref

a306031

rm todos, issue now

11b3af2

lint till I die

7f0fe61

shaspitz commented Aug 15, 2023

View reviewed changes

shaspitz marked this pull request as ready for review August 15, 2023 19:37

shaspitz requested a review from a team as a code owner August 15, 2023 19:37

mpoke requested changes Aug 16, 2023

View reviewed changes

Merge branch 'main' into shawn/clean-protos

2ea9963

github-actions bot added C:Testing Assigned automatically by the PR labeler C:x/consumer Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler C:x/types Assigned automatically by the PR labeler labels Aug 16, 2023

update comments, rebuild protos

c558f55

shaspitz requested a review from mpoke August 16, 2023 16:24

change ccv.go to wire.go, and test file

7986c99

This was referenced Aug 18, 2023

Cleanup x/ccv/types #1213

Closed

Split out consumer genesis state #1214

Closed

Remove deps on external packages for types sent over wire #1215

Closed

insumity reviewed Aug 21, 2023

View reviewed changes

proto/interchain_security/ccv/v1/wire.proto Show resolved Hide resolved

insumity reviewed Aug 21, 2023

View reviewed changes

shaspitz added 3 commits August 21, 2023 13:30

consistent comment

1674152

Update wire.proto

124803a

Merge branch 'main' into shawn/clean-protos

9d67558

insumity reviewed Aug 22, 2023

View reviewed changes

insumity approved these changes Aug 22, 2023

View reviewed changes

example alias diff

065c2cd

mpoke approved these changes Aug 24, 2023

View reviewed changes

shaspitz merged commit 45c7e36 into main Aug 24, 2023
13 of 14 checks passed

shaspitz deleted the shawn/clean-protos branch August 24, 2023 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: clean protos for clarity #1071

refactor: clean protos for clarity #1071

shaspitz commented Jun 22, 2023 •

edited

Loading

shaspitz Jun 22, 2023

mpoke Aug 16, 2023

shaspitz Aug 16, 2023

shaspitz Aug 15, 2023

shaspitz commented Aug 15, 2023 •

edited

Loading

mpoke left a comment

mpoke Aug 16, 2023

mpoke Aug 16, 2023

shaspitz Aug 16, 2023

insumity Aug 21, 2023

shaspitz Aug 21, 2023

insumity Aug 22, 2023

insumity Aug 21, 2023 •

edited

Loading

shaspitz Aug 21, 2023

insumity Aug 21, 2023

shaspitz Aug 21, 2023

insumity Aug 21, 2023

shaspitz Aug 21, 2023 •

edited

Loading

insumity Aug 22, 2023

insumity Aug 22, 2023

shaspitz Aug 22, 2023

insumity Aug 22, 2023

shaspitz Aug 22, 2023

insumity left a comment

refactor: clean protos for clarity #1071

refactor: clean protos for clarity #1071

Conversation

shaspitz commented Jun 22, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaspitz commented Aug 15, 2023 • edited Loading

mpoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insumity Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaspitz Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insumity left a comment

Choose a reason for hiding this comment

shaspitz commented Jun 22, 2023 •

edited

Loading

shaspitz commented Aug 15, 2023 •

edited

Loading

insumity Aug 21, 2023 •

edited

Loading

shaspitz Aug 21, 2023 •

edited

Loading